Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: url parser should throw error on invalid ipv4 #42915

Merged
merged 1 commit into from
May 1, 2022

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 29, 2022

Fixes: #42914

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 29, 2022
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Apr 29, 2022

@nodejs/url

@Trott
Copy link
Member

Trott commented Apr 29, 2022

It looks like this results in many other tests failing?

@anonrig
Copy link
Member Author

anonrig commented Apr 29, 2022

Hi @Trott I'm trying to pinpoint the reason behind why the C++ code does not work.

@Trott
Copy link
Member

Trott commented Apr 29, 2022

Hi @Trott I'm trying to pinpoint the reason behind why the C++ code does not work.

I'm not terribly familiar with that code or frankly C++ in general, plus I'm kinda occupied with other things right now, but a quick git blame on the relevant part of the file and a git shortlog on the file suggest that @TimothyGu or @addaleax might be people who can provide some direction/guidance. Neither of them are as active on the project as they once were, but they're both still around.

@anonrig
Copy link
Member Author

anonrig commented Apr 29, 2022

So, the problem was that when the parser saw more than 4 . character, it just forgot about the input being a ipv4 and just continued parsing the input as a domain. Because of this particular line:

bool is_ipv4;
ParseIPv4Host(decoded.c_str(), decoded.length(), &is_ipv4);
if (is_ipv4)
  return;

Another big issue is around this line. Since, if parse number fails, it should also return a FAILURE, but before it should define is_ipv4 = true.

int64_t n = ParseNumber(mark, pointer);
if (n < 0)
  return;

{
"input": "http://256.256.256.256.256.",
"base": "http://other.com/",
"failure": true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will introduce more failing tests. I've opened an additional issue in order to keep track of spec compliance and fix those issues.

@anonrig
Copy link
Member Author

anonrig commented Apr 30, 2022

@aduh95 can you re-run the failed test?

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented May 1, 2022

Microscopic nit-pick on the commit message. Can be ignored (someone can fix it while landing if they feel strongly about it), but FYI for future stuff: First word should be an imperative verb (so an action). Instead of "should validate...", just "validate....".

@anonrig
Copy link
Member Author

anonrig commented May 1, 2022

Thank you @Trott. You're absolutely right! I'll definitely take this feedback into consideration in my next commit. BTW, one of the test always goes to timeout. Is this a common problem or related to this pull request?

@Trott
Copy link
Member

Trott commented May 1, 2022

one of the test always goes to timeout. Is this a common problem or related to this pull request?

I haven't looked to see what test you are talking about specifically, but test reliability is definitely an issue we struggle with, so likely a common problem.

PR-URL: nodejs#42915
Fixes: nodejs#42914
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@Trott Trott merged commit 87d0d7a into nodejs:master May 1, 2022
@Trott
Copy link
Member

Trott commented May 1, 2022

Landed in 87d0d7a

@Trott
Copy link
Member

Trott commented May 1, 2022

Thanks for the contribution! 🎉

@anonrig anonrig deleted the fix/url-ipv4 branch May 1, 2022 20:37
@anonrig
Copy link
Member Author

anonrig commented May 1, 2022

Thanks @Trott!

@sepehrst
Copy link

sepehrst commented May 2, 2022

@anonrig @aduh95 @targos @Trott This is not the right fix.
It now throws on new URL('http://256.256.256.256.com')
Sorry I don't have the time to open an issue.

@aduh95
Copy link
Contributor

aduh95 commented May 2, 2022

This PR changed quite a lot between the time I reviewed it and the time it landed, and #42915 (comment) looks like a blocker to me. I think we should revert.

aduh95 added a commit to aduh95/node that referenced this pull request May 2, 2022
@targos targos added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label May 2, 2022
nodejs-github-bot pushed a commit that referenced this pull request May 5, 2022
This reverts commit 87d0d7a.

Refs: #42915

PR-URL: #42940
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 11, 2022
PR-URL: #42915
Fixes: #42914
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 11, 2022
This reverts commit 87d0d7a.

Refs: #42915

PR-URL: #42940
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL parser does not validate ipv4 with more than 4 parts
8 participants